Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possibly closes #166 and #158 #336

Merged
merged 1 commit into from
Jan 27, 2017

Conversation

mikfire
Copy link
Contributor

@mikfire mikfire commented Jan 12, 2017

This is a hard thing.

The essence is that in BtLineEdit::setText(BeerXMLElement *element), the conversion went from QVariant to QString to double. The problem was that the QString ended up as something like "1.045". The conversion from QString to double is locale aware, so en_US would get 1.045 but locales like bt_PR would get 1e3+45.

I fixed this by replacing the QVariant->QString->double conversion with a QVariant->double conversion. It seems to work -- I've tried this with en_US, bt_PR, de_DE and fr_FR and they all appear to do the right thing.

I don't think we need to fix anything else. The values written to the database are written as doubles, so this kind of conversion is irrelevant. On the other hand, I have demonstrated an annoying inability to test my own code and my database my not behave like anybody else.

This is a hard thing.

The essence is that in BtLineEdit::setText(BeerXMLElement *element), the
conversion went from QVariant to QString to double. The problem was that the
QString ended up as something like "1.045". The conversion from QString to
double is locale aware, so en_US would get 1.045 but locales like bt_PR would
get 1e3+45.

I fixed this by replacing the QVariant->QString->double conversion with a
QVariant->double conversion. It *seems* to work -- I've tried this with en_US,
bt_PR, de_DE and fr_FR and they all *appear* to do the right thing.

I don't think we need to fix anything else. The values written to the database
are written as doubles, so this kind of conversion is irrelevant. On the other
hand, I have demonstrated an annoying inability to test my own code and my
database my not behave like anybody elses.
@malavv
Copy link
Contributor

malavv commented Jan 12, 2017

As I mention, I wasn't able to replicate on my side so I won't be of great help on this.

However, the code seems alright; lots of formatting changes though. The main function you modified looks better like this. The string middle conversion was a bit weird.

As a side comment, is there a specific reason we use toLatin1 instead of toUtf8 even though I realize that this will logically only be numbers and a few punctuation.

@mikfire
Copy link
Contributor Author

mikfire commented Jan 12, 2017

I have set my editor up to automatically remove trailing white space when I save a file. That tends to cause the formatting changes to happen, but it is really just cleaning up.

I am really uncertain as to why we use toLatin1(). I use it now mostly because I copy/paste the string. I have no idea who used it first (git blame suggests it was me), or why.

@mikfire
Copy link
Contributor Author

mikfire commented Jan 13, 2017

Oh. The only way I could reliably exercise the bug was to say: LC_ALL=pt_BR ./src/brewtarget

I could sometimes see the problem using de_DE or fr_FR, but it was oddly random.

@mikfire mikfire merged commit 92cd88f into Brewtarget:develop Jan 27, 2017
@mikfire mikfire deleted the bugs/international branch April 23, 2021 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants